pybridge: Stop hard-coding endian flag in DBusChannel #20122
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Nykseli
force-pushed
the
pybridge-endianness
branch
from
9772fb1
to
adba42e
Compare
Nykseli
force-pushed
the
pybridge-endianness
branch
from
adba42e
to
eb1f7e6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
| @@ -48,6 +49,8 @@ | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
|
|
|||
| IS_LITTLE_ENDIAN_MACHINE = sys.byteorder == 'little' | |||
There was a problem hiding this comment.
The endianess of DBus messages is independent of the host and can differ per message. There's a flag in every dbus message header which indicates whether it's LE or BE: https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-messages
However, it looks like sd-bus does not support messages with non-native endianess at all currently, so this is currently just broken anyway: systemd/systemd#27363
There was a problem hiding this comment.
But isn't that a strong reason to actually apply this patch? In main it always sends messages in LE, while with this patch it should send it in native endianess.
There was a problem hiding this comment.
The current code is definitely broken and this change makes it more likely to work on BE platforms, but it's not correct either. DBus clients which send messages in non-native endianess remain broken.
There was a problem hiding this comment.
I looked at the documentation and I agree that this solution is not correct, even if it might work in a lot of situations. Getting the endianness from the message header would the reliable way to do it.
I did a proof of concept where I added a new API to sd-bus that could be used in systemd_ctypes to get the endianness from individual messages. I ended up becoming quite busy after doing this PR and I hope I can start working on this soon again.
| flags="<" if flags is not None else None, | ||
| flags=self.endianness if flags is not None else None, |
There was a problem hiding this comment.
I have some trouble understanding what flags does here. I don't see it explained in the sd_bus_message_new_method_call doc.
But still this looks upside down: I'm fairly sure that we don't want to entirely ignore flags here, i.e. on main it should have said
flags="<" if flags is None else flags,
and consequently, in this PR:
flags=self.endianess if flags is None else flags
@allisonkarlitskaya can you please double-check my logic here? Thanks!
There was a problem hiding this comment.
My understanding was that the flags is a cockpit specific thing: https://github.com/cockpit-project/cockpit/blob/main/doc/protocol.md?plain=1#L468-L474
It seems to implemented in the python bridge in the similar way as it's implemented in the C bridge: https://github.com/cockpit-project/cockpit/blob/main/src/bridge/cockpitdbusjson.c#L1122-L1131
There was a problem hiding this comment.
Ah right, thanks. But this pretty much validates my claim that both the old and the new condition here is the wrong way around. So this definitively needs fixing.
|
My 2¢: the bridge ought to be taking care of this when it converts the |
Fix the issue where data is displayed incorrectly on big endian machines when using pybridge.
For example: In network manager, IPs are displayed in reverse.